Make MAD-X pathing robust + add MQT Extraction #558
+1,728
−585
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…cripts for improved clarity and consistency
- Implemented a new module `mqt_extraction.py` for retrieving MQT (Quadrupole Trim) knob values from NXCALS for specified beams and timestamps. - Added helper functions to generate MAD-X MQT variable names and retrieve corresponding values. - Introduced `DateOrStr` class in `iotools.py` for flexible datetime and string parsing. - Created sample output files for MQT extraction tests: `extracted_mqts_b1.str` and `extracted_mqts_b2.str`. - Developed unit tests for MQT extraction functionality in `test_nxcals_mqt_extraction.py`, ensuring correct retrieval and formatting of MQT values. - Added a utility script `generate_mqt_references.py` to copy reference MQT files from CERN data paths for testing purposes.
… of POSIX strings
jgray-19
added
Type: Bug
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…s are relative to model_dir
JoschD
reviewed
Nov 6, 2025
JoschD
reviewed
Nov 6, 2025
JoschD
reviewed
Nov 6, 2025
JoschD
reviewed
Nov 6, 2025
JoschD
reviewed
Nov 6, 2025
JoschD
reviewed
Nov 6, 2025
Comment on lines
462
to
514
|
|
||
| # use absolute paths to force files into twiss_out directory instead of model-dir | ||
| self.jobfile = self.twiss_out.parent.absolute() / f"job.create_{self.twiss_out.stem}.madx" | ||
| self.logfile= self.twiss_out.parent.absolute() / f"job.create_{self.twiss_out.stem}.log" | ||
| self.logfile = self.twiss_out.parent.absolute() / f"job.create_{self.twiss_out.stem}.log" |
Member
There was a problem hiding this comment.
I think in the line 510 you can also use the _get_madx_path() and then skip the absolute() here?
Now comment: # Put job and logfile into the same dir as the corrected twiss, not the model-dir
Contributor
Author
There was a problem hiding this comment.
Maybe have a look at the new changes?
JoschD
reviewed
Nov 6, 2025
JoschD
reviewed
Nov 6, 2025
JoschD
reviewed
Nov 6, 2025
JoschD
reviewed
Nov 6, 2025
fsoubelet
reviewed
Nov 17, 2025
- Change time parsing to return UTC timezone-aware datetime. - Update delta_days parameter type from int to float across multiple functions. - Rename get_mqts to generate_mqt_names for clarity. - Adjust default values and documentation for delta_days. - Modify tests to reflect changes in function names and time handling.
JoschD
requested changes
Dec 3, 2025
omc3/knob_extractor.py
Outdated
Comment on lines
263
to
269
| state={ | ||
| "action": 'store_true', | ||
| "help": ( | ||
| "Prints the state of the StateTracker. " | ||
| "Does not extract anything else." | ||
| ), | ||
| "action": "store_true", | ||
| "help": ("Prints the state of the StateTracker. Does not extract anything else."), | ||
| }, | ||
| output={ | ||
| "type": PathOrStr, | ||
| "help": ( | ||
| "Specify user-defined output path. " | ||
| "This should probably be `model_dir/knobs.madx`" | ||
| "Specify user-defined output path. This should probably be `model_dir/knobs.madx`" |
Member
There was a problem hiding this comment.
well, either that or you should remove the () around the first help-string
| from datetime import datetime | ||
|
|
||
| # import jpype | ||
| from pyspark.sql import SparkSession |
Member
There was a problem hiding this comment.
still not in the requirements!
It's in the optional ones now, but that's not really clear from the code.
As it is a normal pypi package (not an acc-py-package) I'd just put it in the normal requirements? @fsoubelet?
| The extraction uses the underlying `knob_extraction` module with MQT-specific patterns. | ||
| """ | ||
|
|
| from omc3.model.manager import get_accelerator | ||
| from omc3.model.model_creators.lhc_model_creator import ( | ||
| LhcBestKnowledgeCreator, | ||
| # LhcBestKnowledgeCreator, |
Contributor
Author
There was a problem hiding this comment.
No its' not needed if the EXTRACTED_MQTS_FILENAME stays moved, not sure if we reached consensus on that?
JoschD
reviewed
Dec 3, 2025
Comment on lines
512
to
518
| # Take the directory of the twiss output as output dir | ||
| self.jobfile = self.resolve_madx_path( | ||
| self.twiss_out.parent / f"job.create_{self.twiss_out.stem}.madx" | ||
| ) | ||
| self.logfile = self.resolve_madx_path( | ||
| self.twiss_out.parent / f"job.create_{self.twiss_out.stem}.log" | ||
| ) |
Member
There was a problem hiding this comment.
as twiss_out is already resovled, I don't think you need the function again for each of the sub-paths.
…tency across modules
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I apologise sincerely for the mess of this PR, I left my ruff on... Before you start to read the code, I insist that you read below, and consider the method before scrutinising the code.
Making the MAD-X pathing robust (closes #552)
I did not like the approach of setting the model dir to
Path()and getting the MAD-X script and then resetting back. This approach caused bugs and was prone to developer error.Instead, I have now added acwdargument to wherever MAD-X scripts are created. This can be used with a function in the abstract base class_madx_path.Instead I have created a
_madx_pathfunction that handles creating the correct paths to send to MAD-X, assuming it will be run in themodel_dir._madx_pathfunctionThis function first checks if the path is absolute, and passes it to MAD-X as an absolute path.Otherwise, it assumes that you would like to use the path as a relative path to thecwd, which in all cases is themodel_dir, but is nonetheless flexible.This function tries to create a relative path to the model dir using the
Path.relative_tofunction, otherwise creates an absolute path. This in-built function will error if the path is outside of the model_dir.Result
This means that in all cases, we are always using the actual path to the file. In the case of #552, the file path actually exists and is not just relative, should include the file if it exists.
Tests
I have not written any new tests. There is two situations in which the tests should fail before this PR and pass after, however, I have only ensure the tests simply pass.
MQT Extraction (closes #527)
I have also included a module called
nxcalswith a file calledmqt_extraction. In fairness, this should not have been in this PR, but both issues are MQT related. Please let me know if you would prefer this as a script or change the module name and/or usage, I am not compelled by anything.Tests
The tests test that we get the same result as extractions previously done this year by the GUI. They pass on my machine, and I would recommend you running these, as they won't automatically be run by GitHub.